feat(poc,spiral-emit): Add web and cli emitting of YAML OP instruction#32
Conversation
📝 WalkthroughWalkthroughAdds an OP instruction emission pipeline: composer deps and serializers, reflection helpers, RouteParser and WebOperationsEmitter, InstructionEmitter, console/web endpoints (JSON/YAML), HomeController OP annotations, request DTOs migrated to a new FilterInterface, and Taskfile tasks for emit/validation. ChangesOP Instruction Emission Infrastructure
Sequence DiagramsequenceDiagram
participant Client
participant Router
participant RouteParser
participant WebOperationsEmitter
participant InstructionEmitter
participant Response
Client->>Router: GET /op/instruction
Router->>WebOperationsEmitter: getRoutes()
WebOperationsEmitter->>RouteParser: parse(routeKey, Route)
RouteParser->>RouteParser: reflect controller method / read OP attributes
WebOperationsEmitter->>InstructionEmitter: operations[]
InstructionEmitter->>InstructionEmitter: build SerializableInstruction
InstructionEmitter->>Response: toYaml() / return text/yaml response
Response->>Client: 200 text/yaml body
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
poc/spiral-emit/Taskfile.yml (1)
40-40: ⚡ Quick winAvoid suppressing emitter diagnostics during validation.
Line 40 redirects
task emitstderr to/dev/null, which can hide root-cause errors when emission fails. Keep stderr visible (or gate it behind a verbose flag) for faster debugging.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@poc/spiral-emit/Taskfile.yml` at line 40, The current pipeline hides emitter stderr by redirecting "task emit" with "2>/dev/null"; update the Taskfile entry that runs "task emit | tee /dev/stderr | check-jsonschema --schemafile https://thumbrise.github.io/op/reference/instruction.v1.json -" to stop discarding stderr (remove "2>/dev/null") or make the stderr suppression conditional (e.g., wrap the redirection behind a VERBOSE/DEBUG flag) so that emitter diagnostics from "task emit" are visible during validation and can be turned off explicitly when needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@poc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.php`:
- Around line 7-9: The project is missing the spiral/filters dependency used by
the request DTOs (e.g., BreedListRequest implementing
Spiral\Filters\Model\FilterInterface and five other request classes); update
composer.json by adding "spiral/filters": "^3.16.2" to the require section and
then run composer install (or composer update) to pull the package so the
FilterInterface is available at runtime.
In
`@poc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Console/OPInstructionEmitCommand.php`:
- Line 16: Replace the JSON emission in OPInstructionEmitCommand so the CLI
outputs YAML per the feature contract: locate the line using
$this->info(json_encode($emitter->emit())); and change it to emit YAML (for
example using Symfony\Component\Yaml\Yaml::dump($emitter->emit()) or PHP's
yaml_emit) and pass the resulting YAML string to $this->info(); ensure the
emitter->emit() result is serialized to YAML rather than JSON.
In
`@poc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Web/OPInstructionEmitRoute.php`:
- Around line 14-15: The /op/instruction endpoint
(OPInstructionEmitRoute::__invoke) currently returns full instruction payload to
unauthenticated callers; restrict access by adding an authorization check or
environment gate at the top of __invoke: inject/use the framework's security
service (e.g., AuthorizationChecker or Security) and call
isGranted('ROLE_ADMIN') (or check kernel.environment !== 'prod' if you want
non-production only) and return a 403/forbidden Response via ResponseWrapper
when the check fails. Ensure the guard runs before using InstructionEmitter so
unauthenticated/unauthorized callers never receive the payload.
In
`@poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/DocBlockSummaryReader.php`:
- Around line 22-25: Wrap the DocBlockFactory::create($v) call in a try/catch
that catches InvalidArgumentException so parsing errors do not bubble up; if
create() throws, log or ignore the exception and return an empty string (or a
safe fallback) instead of letting the exception crash RouteParser::parse();
modify the block around DocBlockFactory::createInstance() /
DocBlockFactory::create() in DocBlockSummaryReader.php to return a safe fallback
when catching InvalidArgumentException.
In
`@poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/SerializableInstruction.php`:
- Around line 32-36: The code in SerializableInstruction uses json_encode() and
json_decode() without error handling which can return false and cause a
TypeError; update the JSON normalization in the method to call
json_encode($vars, JSON_THROW_ON_ERROR) and json_decode($json, true, 512,
JSON_THROW_ON_ERROR) inside a try/catch(JsonException) block, and in the catch
either rethrow a clearer exception or handle/log the error before
returning/throwing so that recursiveNullFilter($assoc) is only called with a
valid array; reference the SerializableInstruction method where $json and $assoc
are produced and the recursiveNullFilter() call.
- Around line 13-21: The __toString() method currently calls toYaml() which uses
Yaml::dump(...) with DUMP_EXCEPTION_ON_INVALID_TYPE and may throw, so wrap the
call to toYaml() inside a try/catch in SerializableInstruction::__toString(); on
exception return a safe fallback string (e.g. empty string or a JSON-encoded
safe representation from toAssoc()) and optionally log the error. Ensure you do
not change toYaml() behavior (keep Yaml::dump flags) but prevent any exception
from propagating out of __toString() by catching \Throwable and returning the
fallback.
In
`@poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/Internal/RouteParser.php`:
- Around line 31-35: The parse() method in RouteParser.php assumes
$route->getTarget() is an Action and directly reads 'controller'/'action' and
constructs a ReflectionMethod, which will throw for other target types; add a
runtime type check to ensure the target is an instance of Action (or handle
other supported target types) before calling
$this->propertyReader->read($target, 'controller') and
$this->propertyReader->read($target, 'action'), and skip or appropriately
process non-Action targets so ReflectionMethod is only constructed for valid
controller/action pairs; update parse() to branch on instanceof Action (or the
relevant target classes) around the $target, $controller, $action, and
ReflectionMethod usage.
In `@poc/spiral-emit/composer.json`:
- Around line 15-29: The package phpdocumentor/reflection-docblock is needed at
runtime (DocBlockSummaryReader used by RouteParser), so move
"phpdocumentor/reflection-docblock" from require-dev into the top-level
"require" section of composer.json; update composer.json dependencies
accordingly so that DocBlockFactory::createInstance() and DocBlockSummaryReader
are available in production installs (ensure you add the package entry to the
require block and remove it from require-dev to avoid duplication).
In `@poc/spiral-emit/Taskfile.yml`:
- Line 40: The Taskfile task is validating against a live URL; replace the
remote schema URL with a pinned local schema file committed to the repo (e.g.
add a versioned file like schemas/instruction.v1.json), update the Taskfile.yml
command that runs "task emit ... | check-jsonschema --schemafile ..." to point
to that local path instead of
https://thumbrise.github.io/op/reference/instruction.v1.json, and commit the
schema so CI uses the deterministic, versioned local copy; ensure the local
filename matches the version used in the Taskfile (instruction.v1.json) and
update any README/atlas docs if they reference the remote URL.
---
Nitpick comments:
In `@poc/spiral-emit/Taskfile.yml`:
- Line 40: The current pipeline hides emitter stderr by redirecting "task emit"
with "2>/dev/null"; update the Taskfile entry that runs "task emit | tee
/dev/stderr | check-jsonschema --schemafile
https://thumbrise.github.io/op/reference/instruction.v1.json -" to stop
discarding stderr (remove "2>/dev/null") or make the stderr suppression
conditional (e.g., wrap the redirection behind a VERBOSE/DEBUG flag) so that
emitter diagnostics from "task emit" are visible during validation and can be
turned off explicitly when needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b15572a0-5a98-4493-ad80-8d894d78cc41
⛔ Files ignored due to path filters (1)
poc/spiral-emit/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
poc/spiral-emit/Taskfile.ymlpoc/spiral-emit/app/src/Endpoint/Web/HomeController.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BuyDogRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/DogShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.phppoc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Console/OPInstructionEmitCommand.phppoc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Web/OPInstructionEmitRoute.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/DocBlockSummaryReader.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/PropertyReader.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/SerializableInstruction.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/Internal/RouteParser.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/WebOperationsEmitter.phppoc/spiral-emit/composer.json
1ab8b62 to
c2fd17f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@poc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Console/OPInstructionEmitCommand.php`:
- Line 16: The CLI currently calls json_encode without error handling which can
return false silently; in OPInstructionEmitCommand (the command method that
calls $this->info(json_encode($emitter->emit()))), pass JSON_THROW_ON_ERROR to
json_encode and catch JsonException so failures surface and the command returns
self::FAILURE; update the method to json_encode($emitter->emit(),
JSON_THROW_ON_ERROR) inside a try/catch catching JsonException and log the
exception (process/error output) then return self::FAILURE to fail fast.
In `@poc/spiral-emit/Taskfile.yml`:
- Line 40: The current Taskfile pipeline suppresses emit errors via the shell
redirection "2>/dev/null" which hides diagnostics from the "emit" task; update
the Taskfile (the "task emit" pipeline invocation that currently runs: task emit
2>/dev/null | tee /dev/stderr | check-jsonschema ...) to stop discarding stderr
— remove the "2>/dev/null" redirection (or instead forward stderr into the pipe
with "2>&1") so that actual emit errors (container/PHP failures) are visible to
the user and still passed through to tee and check-jsonschema for debugging.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ec3e5101-b150-411a-afe8-3ffcd93f691d
⛔ Files ignored due to path filters (1)
poc/spiral-emit/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
poc/spiral-emit/Taskfile.ymlpoc/spiral-emit/app/src/Endpoint/Web/HomeController.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BuyDogRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/DogShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.phppoc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Console/OPInstructionEmitCommand.phppoc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Web/OPInstructionEmitRoute.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/DocBlockSummaryReader.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/PropertyReader.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/SerializableInstruction.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/Internal/RouteParser.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/WebOperationsEmitter.phppoc/spiral-emit/composer.json
✅ Files skipped from review due to trivial changes (2)
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/WebOperationsEmitter.php
- poc/spiral-emit/app/src/Endpoint/Web/HomeController.php
🚧 Files skipped from review as they are similar to previous changes (3)
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/DocBlockSummaryReader.php
- poc/spiral-emit/composer.json
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/PropertyReader.php
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
poc/spiral-emit/app/src/Endpoint/Web/HomeController.php (1)
48-60:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAlign
OPErrorterm with the actual thrown exception.Line 49 declares
TestException, but Line 60 throwsException. This makes emitted instruction metadata inaccurate for consumers relying on error contracts.Suggested fix
- #[OPError([ - new Term('TestException', 'This is a test exception.', trait: [ + #[OPError([ + new Term('Exception', 'This is a test exception.', trait: [ new Status('500'), ]), ])]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@poc/spiral-emit/app/src/Endpoint/Web/HomeController.php` around lines 48 - 60, The OPError metadata in HomeController::exception() declares a Term('TestException') but the method actually throws PHP's base Exception, so update the error contract to match the real thrown type (or change the thrown exception to the declared type); specifically, either replace Term('TestException', ...) with Term('Exception', ...) in the #[OPError(...)] attribute for the exception() method, or throw new TestException('This is a test exception.') in exception() so the #[OPError] term, Status('500'), and the thrown exception are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@poc/spiral-emit/app/src/Endpoint/Web/HomeController.php`:
- Around line 48-60: The OPError metadata in HomeController::exception()
declares a Term('TestException') but the method actually throws PHP's base
Exception, so update the error contract to match the real thrown type (or change
the thrown exception to the declared type); specifically, either replace
Term('TestException', ...) with Term('Exception', ...) in the #[OPError(...)]
attribute for the exception() method, or throw new TestException('This is a test
exception.') in exception() so the #[OPError] term, Status('500'), and the
thrown exception are consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2db953f1-a199-42dd-a8b9-0e7d47f3b706
⛔ Files ignored due to path filters (1)
poc/spiral-emit/composer.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
poc/spiral-emit/Taskfile.ymlpoc/spiral-emit/app/src/Endpoint/Web/HomeController.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/BuyDogRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/DogShowRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderListRequest.phppoc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.phppoc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Console/OPInstructionEmitCommand.phppoc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Web/OPInstructionEmitRoute.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/InstructionEmitter.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/DocBlockSummaryReader.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/PropertyReader.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/SerializableInstruction.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/Internal/RouteParser.phppoc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/WebOperationsEmitter.phppoc/spiral-emit/composer.json
🚧 Files skipped from review as they are similar to previous changes (14)
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderCancelRequest.php
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/PropertyReader.php
- poc/spiral-emit/composer.json
- poc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Web/OPInstructionEmitRoute.php
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/SerializableInstruction.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/BuyDogRequest.php
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Internal/Reflection/DocBlockSummaryReader.php
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/WebOperationsEmitter.php
- poc/spiral-emit/Taskfile.yml
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderShowRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/OrderListRequest.php
- poc/spiral-emit/app/src/Endpoint/Web/Request/BreedListRequest.php
- poc/spiral-emit/app/src/Modules/OPSpiral/Infrastructure/Web/Internal/RouteParser.php
- poc/spiral-emit/app/src/Modules/OPSpiral/Endpoints/Console/OPInstructionEmitCommand.php
Summary by CodeRabbit
New Features
Improvements
Chores